Fix LLMJudgeMetric.evaluate crash inside a running event loop#218
Conversation
|
@Nitjsefnie update your branch , there is merge conflict with the main branch |
|
/oc review pr |
|
All 4 tests pass. Here's my review: Review: Fix LLMJudgeMetric.evaluate crash inside a running event loopFiles changed: What it doesAdds What I like
Minor nitThe VerdictApproved. Clean fix, well-tested, consistent with existing patterns, and the regression test correctly reproduces the issue. The merge conflict with main should be straightforward to resolve (just the new |
LLMJudgeMetric.evaluate() drove an async provider's coroutine with asyncio.get_event_loop().run_until_complete(). When evaluate() is called from inside an already-running event loop (the async evaluation pipeline: Pipeline._run_metrics -> _evaluate_item), that raises RuntimeError, which is swallowed by the generic except and silently returns score 0.0 with a misleading "LLM judge failed" reason (issue OpenAgentHQ#71, same pattern as OpenAgentHQ#49). Add a _run_coroutine_blocking() helper that detects a running loop via asyncio.get_running_loop() and, when one is present, runs the coroutine to completion in a separate worker thread that owns its own event loop (concurrent.futures) — mirroring the existing precedent in openagent_eval.cicd.plugin. With no running loop it drives the coroutine directly via asyncio.run(). The sync-provider path is unchanged. Regression tests cover the coroutine provider from both a plain sync context and from inside asyncio.run(...), the sync-provider path, and the missing-input short-circuit. Fixes OpenAgentHQ#71 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1236098 to
2d98e3e
Compare
|
/oc review pr |
Review: Fix
|
|
🎉 Congratulations @Nitjsefnie! Your pull request has been successfully merged into main. 🚀 Thank you for contributing to OpenAgentHQ and helping improve the project. We truly appreciate your contribution and hope to see you back with more amazing PRs! Happy Open Sourcing! ❤️ |
2 participants
Fixes #71
LLMJudgeMetric.evaluate()drove an async provider's coroutine withasyncio.get_event_loop().run_until_complete(), which raisesRuntimeErrorwhen called from inside an already-running loop (the async evaluation pipeline) — and theexceptswallowed it into a silent0.0score.The fix adds
_run_coroutine_blocking(): no running loop →asyncio.run()as before; running loop present → the coroutine executes in a worker thread with its own event loop. This mirrors the repo's existing pattern inopenagent_eval/cicd/plugin.py(ThreadPoolExecutor +asyncio.run), withget_running_loop()try/except for detection (avoids theget_event_loop()deprecation). I deliberately didn't useasyncio.run_coroutine_threadsafe(coro, loop).result()— it schedules onto the same loop whose thread is blocked in the synchronousevaluate()call, which deadlocks. Happy to rework toward a shared internal async path instead if you'd prefer that shape.Regression tests cover both contexts; the running-loop test fails on current main with exactly the reported symptom (
LLM judge failed: This event loop is already running→ score 0.0). Note the message wording differs by Python version (3.13 says "This event loop is already running"; the issue quotes the 3.11/3.12 wording) — the test asserts behavior, not the string.Gates:
uv run pytest tests/unit— 938 passed, 4 skipped (pre-existing chromadb skips).Generated by Claude Fable 5 (brief, review), Claude Opus 4.8 (implementation)